Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add encodeUriComponent to file name #967

Merged
merged 5 commits into from
Jan 20, 2019
Merged

Conversation

jorgerobles
Copy link
Contributor

Reasons for making this change

Fix for non standard file names (with spaces, apostrophes, etc) that prevent to be used.
For example uploading a whitespace named file prevents to be used as background url.

Checklist

  • I'm updating documentation
    • I've checked the rendering of the Markdown text I've added
    • If I'm adding a new section, I've updated the Table of Content
  • I'm adding or updating code
    • I've added and/or updated tests
    • I've updated docs if needed
    • I've run npm run cs-format on my branch to conform my code to prettier coding style
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

Fix for non standard file names (with spaces, apostrophes, etc) that prevent to be used.
@@ -4,6 +4,7 @@ import PropTypes from "prop-types";
import { dataURItoBlob, shouldRender, setState } from "../../utils";

function addNameToDataURL(dataURL, name) {
name=encodeURIComponent(name)
Copy link
Contributor

@graingert graingert Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you've got a lint error here.

Do this instead:

  function addNameToDataURL(dataURL, name) {
    return dataURL.replace(";base64", `;name=${encodeURIComponent(name)};base64`); 
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @graingert, did not see your comment until now.

Copy link
Member

@epicfaace epicfaace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorgerobles Thanks for the PR! Do you still plan to work on this? In addition to the lint error, I would suggest you also add a test.

Copy link
Contributor Author

@jorgerobles jorgerobles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to fix it 🤦‍♂️

@epicfaace
Copy link
Member

Great, thanks @jorgerobles ! Can you also add a test? It would be similar to the tests already existing: https://github.com/mozilla-services/react-jsonschema-form/blob/1941e760d8af93b118bbc193dd98b88f64b32645/test/StringField_test.js#L1503

@jorgerobles
Copy link
Contributor Author

@epicfaace I've added the test. I'm not very familiarised with mocha, hope is well.

@epicfaace
Copy link
Member

@jorgerobles Thanks, almost there -- instead of modifying the existing test, can you leave the existing test case "should reflect the change into the dom" in place and add a new test case right next to it? (you can copy and paste the existing test case and make the changes that you did)

@@ -1501,6 +1501,9 @@ describe("StringField", () => {
describe("FileWidget", () => {
const initialValue = "data:text/plain;name=file1.txt;base64,dGVzdDE=";

const nonUriEncodedValue = "fileáéí óú1.txt";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move these two lines inside the "should encode file name with encodeURIComponent" test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@epicfaace epicfaace merged commit 75571ba into rjsf-team:master Jan 20, 2019
@epicfaace
Copy link
Member

Thank you, @jorgerobles !

@jorgerobles
Copy link
Contributor Author

jorgerobles commented Jan 20, 2019 via email

@epicfaace
Copy link
Member

@jorgerobles can you help with #824? It's pretty similar to this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants